feat(sentry): add SentrySdkAspect for coroutine-safe SDK usage#1063
feat(sentry): add SentrySdkAspect for coroutine-safe SDK usage#1063huangdijia wants to merge 5 commits intomainfrom
Conversation
Add an aspect that intercepts SentrySdk::init, ::setCurrentHub, and ::getRuntimeContextManager to make them coroutine-safe using Hyperf's Context API and RuntimeContextManager. This ensures that Sentry SDK works correctly in a coroutine environment by storing the runtime context manager in Hyperf's context storage.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
总览此变更将 Sentry PHP 包依赖版本从 ^4.19.0 升级至 ^4.21.0，并引入新的 AOP 切面类拦截 Sentry SDK 方法调用，整合至 Hyperf 上下文管理系统中。 变更
序列图sequenceDiagram
participant App as 应用程序
participant Aspect as SentrySdkAspect
participant SDK as SentrySdk
participant Context as Hyperf Context
participant Manager as RuntimeContextManager
participant Hub as HubInterface
App->>Aspect: 调用 SentrySdk::init()
Aspect->>Hub: 创建 Hub 实例
Aspect->>Manager: 创建 RuntimeContextManager
Aspect->>Context: 存储 Manager 至 Context
Aspect->>App: 返回 Hub
App->>Aspect: 调用 SentrySdk::setCurrentHub(hub)
Aspect->>Context: 获取 RuntimeContextManager
Aspect->>Manager: 更新当前 Hub
Aspect->>App: 返回 Hub
App->>Aspect: 调用 SentrySdk::getRuntimeContextManager()
Aspect->>Context: 查询 Context
alt Manager 存在
Aspect->>Manager: 返回已存储的 Manager
else Manager 不存在
Aspect->>Hub: 创建新 Hub 实例
Aspect->>Manager: 创建新 RuntimeContextManager
Aspect->>Context: 存储 Manager 至 Context
end
Aspect->>App: 返回 Manager
代码审查工作量估计🎯 3 (中等) | ⏱️ ~25 分钟 相关 PR
建议审核人
兔兔诗
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This pull request attempts to add coroutine-safety to Sentry SDK usage by introducing a SentrySdkAspect that intercepts SentrySdk methods and uses Hyperf's Context API with RuntimeContextManager. However, this approach is fundamentally flawed because the codebase already implements coroutine-safety for SentrySdk through a class_map override (registered in ConfigProvider line 80, implemented in class_map/SentrySdk.php).
Changes:
- Added
SentrySdkAspectclass to intercept SentrySdk methods - Registered the aspect in ConfigProvider
- Attempted to use RuntimeContextManager for coroutine isolation
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| src/sentry/src/ConfigProvider.php | Registers the new SentrySdkAspect (conflicts with existing class_map) |
| src/sentry/src/Aspect/SentrySdkAspect.php | New aspect attempting to make SentrySdk coroutine-safe (redundant and incompatible) |
| $arguments = $proceedingJoinPoint->arguments ?? []; | ||
| $hub = $arguments['hub']; |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/sentry/src/Aspect/SentrySdkAspect.php (1)
43-43: 可选：移除未使用形参，消除静态分析噪音。
handleInit()与handleGetRuntimeContextManager()的$proceedingJoinPoint未被使用，可删掉以减少 PHPMD 告警。♻️ 建议修改
- 'init' => $this->handleInit($proceedingJoinPoint), + 'init' => $this->handleInit(), 'setCurrentHub' => $this->handleSetCurrentHub($proceedingJoinPoint), - 'getRuntimeContextManager' => $this->handleGetRuntimeContextManager($proceedingJoinPoint), + 'getRuntimeContextManager' => $this->handleGetRuntimeContextManager(), - private function handleInit(ProceedingJoinPoint $proceedingJoinPoint) + private function handleInit() { Context::set( RuntimeContextManager::class, new RuntimeContextManager(make(HubInterface::class)) ); return SentrySdk::getCurrentHub(); } - private function handleGetRuntimeContextManager(ProceedingJoinPoint $proceedingJoinPoint) + private function handleGetRuntimeContextManager() { return Context::getOrSet( RuntimeContextManager::class, fn () => new RuntimeContextManager(make(HubInterface::class)) ); }Also applies to: 63-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sentry/src/Aspect/SentrySdkAspect.php` at line 43, Both handleInit() and handleGetRuntimeContextManager() declare an unused ProceedingJoinPoint parameter causing static-analysis noise; remove the unused parameter from both method signatures (e.g., change "private function handleInit(ProceedingJoinPoint $proceedingJoinPoint)" and "private function handleGetRuntimeContextManager(ProceedingJoinPoint $proceedingJoinPoint)" to signatures without parameters), ensure there are no internal references to $proceedingJoinPoint, and update any callers or pointcut/advice mappings if they reference the method signature so the aspect wiring remains correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/sentry/src/Aspect/SentrySdkAspect.php`:
- Line 43: Both handleInit() and handleGetRuntimeContextManager() declare an
unused ProceedingJoinPoint parameter causing static-analysis noise; remove the
unused parameter from both method signatures (e.g., change "private function
handleInit(ProceedingJoinPoint $proceedingJoinPoint)" and "private function
handleGetRuntimeContextManager(ProceedingJoinPoint $proceedingJoinPoint)" to
signatures without parameters), ensure there are no internal references to
$proceedingJoinPoint, and update any callers or pointcut/advice mappings if they
reference the method signature so the aspect wiring remains correct.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
composer.jsonsrc/sentry/composer.jsonsrc/sentry/src/Aspect/SentrySdkAspect.phpsrc/sentry/src/ConfigProvider.php
| class SentrySdkAspect extends AbstractAspect | ||
| { | ||
| public array $classes = [ | ||
| SentrySdk::class . '::init', | ||
| SentrySdk::class . '::setCurrentHub', | ||
| SentrySdk::class . '::getRuntimeContextManager', | ||
| ]; | ||
|
|
||
| public function process(ProceedingJoinPoint $proceedingJoinPoint) | ||
| { | ||
| // Do nothing, just for class mapping. | ||
| return match ($proceedingJoinPoint->methodName) { | ||
| 'init' => $this->handleInit($proceedingJoinPoint), | ||
| 'setCurrentHub' => $this->handleSetCurrentHub($proceedingJoinPoint), | ||
| 'getRuntimeContextManager' => $this->handleGetRuntimeContextManager($proceedingJoinPoint), | ||
| default => $proceedingJoinPoint->process(), | ||
| }; | ||
| } | ||
|
|
||
| private function handleInit(ProceedingJoinPoint $proceedingJoinPoint) | ||
| { | ||
| Context::set( | ||
| RuntimeContextManager::class, | ||
| new RuntimeContextManager(make(HubInterface::class)) | ||
| ); | ||
|
|
||
| return SentrySdk::getCurrentHub(); | ||
| } | ||
|
|
||
| private function handleSetCurrentHub(ProceedingJoinPoint $proceedingJoinPoint) | ||
| { | ||
| $arguments = $proceedingJoinPoint->arguments ?? []; | ||
| $hub = $arguments['hub']; | ||
| // @phpstan-ignore-next-line | ||
| Closure::bind(fn () => static::getRuntimeContextManager()->setCurrentHub($hub), null, SentrySdk::class)(); | ||
|
|
||
| return $hub; | ||
| } | ||
|
|
||
| private function handleGetRuntimeContextManager(ProceedingJoinPoint $proceedingJoinPoint) | ||
| { | ||
| return Context::getOrSet( | ||
| RuntimeContextManager::class, | ||
| fn () => new RuntimeContextManager(make(HubInterface::class)) | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
补齐协程并发隔离测试，当前改动缺少回归保护。
该文件改动直接接管 SentrySdk 的核心生命周期方法，但本 PR 未新增并发协程下的隔离测试（多协程 init/setCurrentHub/getRuntimeContextManager 互不污染）。建议在合并前补齐至少一组并发回归用例。
我可以直接给你补一版 Pest 用例草稿（含并发场景和跨请求隔离断言），要我顺手起一个测试任务吗？
As per coding guidelines "Maintain type coverage by updating or adding tests when public APIs change; ensure composer test:types stays green before pushing".
🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 43-43: Avoid unused parameters such as '$proceedingJoinPoint'. (undefined)
(UnusedFormalParameter)
[warning] 63-63: Avoid unused parameters such as '$proceedingJoinPoint'. (undefined)
(UnusedFormalParameter)
Summary
SentrySdkAspectto make Sentry SDK methods coroutine-safe using Hyperf's Context APISentrySdk::init,::setCurrentHub, and::getRuntimeContextManagerRuntimeContextManagerstored in Hyperf's context for proper coroutine isolationTest plan
Summary by CodeRabbit
Release Notes
依赖更新
改进